Skip to content

Solution#173

Open
vilich-mykhailo wants to merge 3 commits intomate-academy:masterfrom
vilich-mykhailo:develop
Open

Solution#173
vilich-mykhailo wants to merge 3 commits intomate-academy:masterfrom
vilich-mykhailo:develop

Conversation

@vilich-mykhailo
Copy link

No description provided.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

Good work setting up the server and handling the POST request to save the expense data. However, a couple of key requirements are missing, so this will need a few changes before it can be approved.

Here are the two main issues:

  1. Missing HTML Form: The application must show an HTML form for users to input data. Your server currently only handles POST requests. You'll need to add a handler for GET requests that serves the HTML form file.
  2. Incorrect Success Response: The server is returning raw JSON after a successful submission. The requirement is to return an "HTML page with well formatted JSON". To fix this, please set the Content-Type header to text/html and wrap the JSON response within an HTML structure, for example, inside a <pre> tag.

You're very close to a complete solution! Address these points and you'll be all set.


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Important

Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.

Tip

If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.

Caution

Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.

Footnotes

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

// Return instance of http.Server class
const server = new Server();

server.on('request', (req, res) => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The request handler is missing a crucial part of the requirements. The application must also 'show an HTML form'. You should add logic here to handle GET requests to serve this form to the user.

Comment on lines 64 to 65
res.setHeader('Content-Type', 'application/json');
res.end(JSON.stringify(expense, null, 2));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the task description, the server should 'return an HTML page with well formatted JSON'. This implementation returns raw JSON. You should set the Content-Type to text/html and wrap the JSON string in HTML tags (e.g., inside a <pre> tag).

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

Great job adding the index.html file! This is a big step forward. However, the server isn't yet configured to use it, and there are a few critical issues with data handling that prevent the application from working as required, so some changes are needed.

Here are the main things to address:

  1. Serve the HTML Form: The server needs to handle GET requests to the root path (/) and respond with the content of your index.html file so users can see the form.
  2. Parse Form Data: Your server expects JSON, but the HTML form sends data as application/x-www-form-urlencoded. This will cause a crash when you use JSON.parse(). You'll need to parse this form data first—Node's built-in URLSearchParams is great for this.
  3. Save Valid JSON: After parsing the form data into an object, you must use JSON.stringify() to convert it back into a JSON string before writing it to db/expense.json.
  4. Return an HTML Page: On a successful submission, the server must send back an HTML document that displays the saved JSON, not just the raw JSON data. Remember to set the Content-Type header to text/html.

You're very close to a complete solution. Keep up the great work!


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Important

Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.

Tip

If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.

Caution

Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.

Footnotes

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

const server = new http.Server();

server.on('request', (req, res) => {
if (req.url !== '/add-expense') {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This routing logic only checks for a single URL and doesn't distinguish between HTTP methods like GET and POST. To meet the requirement of showing an HTML form, you'll need to handle GET requests (for example, to the root path /) to serve the index.html file. The form submission itself should be handled on a POST request to /add-expense.

Comment on lines 28 to 39
const data = JSON.parse(text);

if (!data['date'] || !data['title'] || !data['amount']) {
res.statusCode = 400;
res.end('Not full data');

return;
}

const writeStream = fs.createWriteStream(path.resolve('db/expense.json'));

writeStream.end(text);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The server expects JSON, but a standard HTML form sends data in application/x-www-form-urlencoded format. JSON.parse(text) will cause an error with this data. You'll need to parse the urlencoded string first (you can use the built-in URLSearchParams class or the querystring module). After parsing, you should construct a JavaScript object from the form data, stringify it to JSON, and then save that JSON string to the file, not the original raw text from the request.

Comment on lines 42 to 44
res.setHeader('Content-type', 'application/json');
res.statusCode = 200;
res.end(text);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The requirements state that the server must return an HTML page with the formatted JSON. The Content-Type should be text/html, and the response body should be an HTML document containing the JSON data (for example, inside a <pre> tag to preserve formatting). This was also mentioned in the previous review feedback.

src/index.html Outdated
</head>
<body>
<h1>Expense Form</h1>
<form action="http://localhost:5701/add-expense" method="POST">

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This form submits data in application/x-www-form-urlencoded format by default. However, your server code in createServer.js expects application/json because it uses JSON.parse(). To fix this, you'll need to use JavaScript to intercept the form's submit event, create a JSON object from the form data, and then send it using the Fetch API with the correct Content-Type header.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I am approving your solution as an educational override so you can continue making progress. You've done a great job setting up the server to display the HTML form! However, the form submission is not yet working due to a couple of key issues.

The main problem is that the server tries to parse the form data as JSON, but it's sent in application/x-www-form-urlencoded format. You'll need to use a different method to parse this, such as URLSearchParams. Secondly, the success response after saving the data should be a full HTML page displaying the saved JSON, with the Content-Type header set to text/html.

Addressing these two points will make your application fully functional. Keep up the fantastic effort!


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Footnotes

  1. Rate AI review example

let fields;

try {
fields = JSON.parse(body);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This parsing method isn't correct for the data sent by the HTML form. Forms using POST send data in application/x-www-form-urlencoded format (like date=...&title=...), which is not valid JSON. JSON.parse() will throw an error here.

You'll need to parse this string differently. Consider using Node's built-in URLSearchParams to handle this format.

Comment on lines +50 to +51
res.setHeader('Content-Type', 'application/json');
res.end(file);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The response for a successful submission needs two changes to meet the requirements:

  1. The Content-Type header should be text/html.
  2. The response body should be an HTML document that displays the saved JSON, not the raw JSON data itself. For example: res.end(
    ${file}
    );

return;
}

if (req.url !== '/' && req.url !== '/add-expense') {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This condition correctly routes GET / requests to show the form, but it also allows GET /add-expense to do the same, which is likely not intended. It's generally better to handle each route explicitly. For instance, you could check for req.method === 'GET' && req.url === '/' to serve the form, and then let other unhandled routes result in a 404.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants